cli added config support and validate command
#729
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
March 2, 2024 12:11
Bit of scope creep. Changes: - drop support .ini config because they don't have a basic construct for lists, which is required for final_vars in driver.validate_execution() - rename config methods to context. It allows to reduce the number of arguments to pass to the CLI and containts Driver config + execution final_vars, inputs, and overrides - refactored the CLI to extract methods to try a command and to display a result in terminal. - added a validate command. While build checks for a valid definition, validate checks that certain dataflow paths aren't broken for a given context.
skrawcz
reviewed
Mar 3, 2024
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me!
- Reviewed the entire pull request up to b3fd011
- Looked at
679lines of code in5files - Took 2 minutes and 5 seconds to review
More info
- Skipped
1files when reviewing. - Skipped posting
4additional comments because they didn't meet confidence threshold of50%.
1. hamilton/cli/logic.py:268:
- Assessed confidence :
80% - Grade:
0% - Comment:
Consider adding validation to ensure that the loaded configuration contains all the required fields and that they are in the correct format. This could help prevent unexpected errors and make the code more robust. - Reasoning:
Theload_contextfunction inlogic.pyis responsible for loading the configuration from a file. It currently supports.jsonand.pyfiles. However, there is no validation to ensure that the loaded configuration is in the correct format. This could lead to unexpected errors if the configuration file is malformed or missing required fields.
2. hamilton/cli/commands.py:82:
- Assessed confidence :
80% - Grade:
0% - Comment:
Consider adding error handling for thevalidate_executionmethod call. This could help provide more informative error messages and make the code more robust. - Reasoning:
Thevalidatefunction incommands.pyis responsible for validating the execution of a dataflow. It uses thevalidate_executionmethod of theDriverclass, passing in the final variables, inputs, and overrides from the context. However, there is no error handling in case thevalidate_executionmethod raises an exception. This could lead to unhandled exceptions if there are issues with the validation.
3. hamilton/cli/commands.py:30:
- Assessed confidence :
80% - Grade:
0% - Comment:
Consider adding error handling for theload_modules_from_gitfunction call. This could help provide more informative error messages and make the code more robust. - Reasoning:
Thedifffunction incommands.pyis responsible for generating a diff between the current modules and a specified git reference. It uses theload_modules_from_gitfunction to load the modules from the git reference. However, there is no error handling in case theload_modules_from_gitfunction raises an exception. This could lead to unhandled exceptions if there are issues with loading the modules.
4. hamilton/cli/commands.py:8:
- Assessed confidence :
80% - Grade:
0% - Comment:
Consider adding validation to ensure that the loaded modules are in the correct format. This could help prevent unexpected errors and make the code more robust. - Reasoning:
Thebuildfunction incommands.pyis responsible for building a Hamilton driver from the passed modules and loading the Driver config from the context file. However, there is no validation to ensure that the loaded modules are in the correct format. This could lead to unexpected errors if the modules are malformed or missing required fields.
Workflow ID: wflow_N88WFOK8NSTL9YvQ
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
zilto
changed the title
cli load config from .ini, .json, .pycli added config support and validate command
There was a problem hiding this comment.
❌ Changes requested.
- Performed an incremental review on 0405587
- Looked at
13lines of code in1files - Took 2 minutes and 32 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
0additional comments because they didn't meet confidence threshold of50%.
Workflow ID: wflow_BhUEU8zUd1zAXidY
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on 409e996
- Looked at
13lines of code in1files - Took 2 minutes and 28 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
0additional comments because they didn't meet confidence threshold of50%.
Workflow ID: wflow_ZILROIimfIFDt4nZ
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on 4a07446
- Looked at
58lines of code in1files - Took 2 minutes and 30 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. /hamilton/cli/__main__.py:77:
- Assessed confidence :
100% - Grade:
0% - Comment:
The PR description mentions that the CLI now accepts a--configoption, but I don't see this option being handled in this file. Please ensure that this option is properly implemented and handled. - Reasoning:
The PR description mentions that the CLI now accepts a--configoption, but I don't see this option being handled in the__main__.pyfile. This could be an oversight or the changes might be in another file. I'll need to check the other files in the PR to confirm.
Workflow ID: wflow_BDxtopp5sMKtpog8
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
I think the CLI is still super experimental. As I work with pre-commit and GitHub actions, new API needs appear and things change frequently. We should strongly emphasize that user rely on Generally speaking, we should probably start adding dates to |
There was a problem hiding this comment.
❌ Changes requested.
- Performed an incremental review on 42e236d
- Looked at
23lines of code in1files - Took 4 minutes and 2 seconds to review
More info
- Skipped
1files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. examples/cli/module_v1.py:15:
- Assessed confidence :
100% - Grade:
0% - Comment:
Consider passing the file path as a parameter to the function instead of hardcoding it. This would make the function more flexible and prevent potential issues if the file is not found at the specified location. - Reasoning:
The functioncustomers_df__defaultis reading a CSV file from a hardcoded path. This could lead to issues if the file is not found at the specified location. It would be better to pass the file path as a parameter to the function.
Workflow ID: wflow_Do1w7MRGxUTaS3TS
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on 692fb0d
- Looked at
35lines of code in1files - Took 2 minutes and 58 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. /examples/cli/README.md:13:
- Assessed confidence :
0% - Comment:
The changes in the README file are clear and well-documented. Good job! - Reasoning:
The README file has been updated to reflect the new changes in the CLI. The changes are well-documented and clear. There are no issues with the changes in the README file.
Workflow ID: wflow_MH1S1Mkp9mFxpxyF
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on 427f523
- Looked at
745lines of code in6files - Took 9 minutes and 13 seconds to review
More info
- Skipped
1files when reviewing. - Skipped posting
5additional comments because they didn't meet confidence threshold of50%.
1. /hamilton/cli/logic.py:268:
- Assessed confidence :
100% - Grade:
0% - Comment:
Theload_contextfunction should handle the case where the file does not exist. Consider adding error handling forFileNotFoundError. - Reasoning:
Theload_contextfunction inlogic.pyis loading the context from a file. However, it does not handle the case where the file does not exist. This could lead to aFileNotFoundErrorwhich is not being caught and handled. This could be improved by adding error handling for this case.
2. /hamilton/cli/logic.py:269:
- Assessed confidence :
100% - Grade:
0% - Comment:
Theload_contextfunction should handle the case where the file does not exist. Consider adding error handling forFileNotFoundError. - Reasoning:
Theload_contextfunction inlogic.pyis loading the context from a file. However, it does not handle the case where the file does not exist. This could lead to aFileNotFoundErrorwhich is not being caught and handled. This could be improved by adding error handling for this case.
3. /hamilton/cli/logic.py:278:
- Assessed confidence :
100% - Grade:
0% - Comment:
Theload_contextfunction should handle the case where the file extension is not supported. Consider adding error handling forValueError. - Reasoning:
Theload_contextfunction inlogic.pyis loading the context from a file. However, it does not handle the case where the file extension is not supported. This could lead to aValueErrorwhich is not being caught and handled. This could be improved by adding error handling for this case.
4. /hamilton/cli/logic.py:270:
- Assessed confidence :
100% - Grade:
0% - Comment:
Theload_contextfunction should handle the case where the file does not exist. Consider adding error handling forFileNotFoundError. - Reasoning:
Theload_contextfunction inlogic.pyis loading the context from a file. However, it does not handle the case where the file does not exist. This could lead to aFileNotFoundErrorwhich is not being caught and handled. This could be improved by adding error handling for this case.
5. /hamilton/cli/logic.py:279:
- Assessed confidence :
100% - Grade:
0% - Comment:
Theload_contextfunction should handle the case where the file extension is not supported. Consider adding error handling forValueError. - Reasoning:
Theload_contextfunction inlogic.pyis loading the context from a file. However, it does not handle the case where the file extension is not supported. This could lead to aValueErrorwhich is not being caught and handled. This could be improved by adding error handling for this case.
Workflow ID: wflow_rJZzWw2DQqGXJ0bG
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now, all available commands accept
--configwhich receives aPath. The path can be a.json,.py, or.inifile and the content is returned as a dictionary.Changes
node.originating_functions is Nonefor config nodescommands.pyto load config before building the driver__main__.pyfor the CLI to handle a new optionHow I tested this
examples/clifolder and manually tried them.Notes
❗ I will add a
hamilton validatecommand then update docs at onceUnionof types for arguments. Would need to add an additional--config-json-cjsonto each command and prevent conflicts with--config. Maybe things could be renamed to--config-pathand--configwhen received as JSON stringhamilton build key1="true" key2=0 MODULES, but it would be messier to parse and maintainChecklist
Summary:
This PR enhances the CLI by adding support for configuration files, handling a new case for config nodes, updating the driver building process, handling a new CLI option, improving error handling, adding a
validatecommand, and providing a demonstration of the new configuration options.Key points:
node.originating_functionsisNonefor config nodescommands.pyto load the config before building the driver__main__.pyfor the CLI to handle a new optionvalidatecommand to the CLImodule_v1.pyin theexamples/clidirectoryGenerated with ❤️ by ellipsis.dev